Revert "refactor: use Cmd class for hook command execution"#1690
Revert "refactor: use Cmd class for hook command execution"#1690dengbo11 wants to merge 1 commit into
Conversation
This reverts commit 9e3220f.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the custom Cmd execution mechanism in executeHookCommands with std::system and uses EnvironmentVariableGuard to manage environment variables. It also introduces a helper function escapeAndWrapCommandForShell to escape and wrap command strings. Feedback on this PR highlights a critical bug in escapeAndWrapCommandForShell where the command string is not wrapped in single quotes, which breaks commands with arguments when passed to sh -c. The reviewer suggested either removing the redundant sh -c wrapping entirely (since std::system already invokes the shell) or properly wrapping the escaped command in single quotes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| static std::string escapeAndWrapCommandForShell(const std::string &command) | ||
| { | ||
| std::string escapedCommand = command; | ||
| size_t pos = 0; | ||
| // Replace ' with '\'' (close current quote, add literal single quote, open new quote) | ||
| while ((pos = escapedCommand.find('\'', pos)) != std::string::npos) { | ||
| escapedCommand.replace(pos, 1, "'\\''"); | ||
| pos += 4; | ||
| } | ||
|
|
||
| return "sh -c " + escapedCommand; | ||
| } |
There was a problem hiding this comment.
The escapeAndWrapCommandForShell function is missing single quotes around the command string in its return statement. Currently, it returns "sh -c " + escapedCommand.
Without single quotes, a command with arguments (e.g., echo foo bar) is passed to std::system as sh -c echo foo bar. The outer shell executes sh -c echo foo bar, which runs sh with -c and echo as the command string, treating foo and bar as positional parameters ($0, $1) for the inner shell. Since the command string echo does not reference these positional parameters, they are completely ignored, and the command runs without arguments (e.g., printing an empty line instead of foo bar). This completely breaks any hook command that contains arguments.
Recommended Solutions:
-
Simplest & Best Approach: Completely remove
escapeAndWrapCommandForShelland passcommand_rawdirectly tostd::system. Sincestd::systemalready executes the command string by invoking/bin/sh -c, wrapping it in anothersh -cis redundant, spawns an extra shell process, and introduces unnecessary escaping complexity. -
Alternative (if explicit wrapping is required): Wrap the escaped command in single quotes as shown in the suggestion below.
| static std::string escapeAndWrapCommandForShell(const std::string &command) | |
| { | |
| std::string escapedCommand = command; | |
| size_t pos = 0; | |
| // Replace ' with '\'' (close current quote, add literal single quote, open new quote) | |
| while ((pos = escapedCommand.find('\'', pos)) != std::string::npos) { | |
| escapedCommand.replace(pos, 1, "'\\''"); | |
| pos += 4; | |
| } | |
| return "sh -c " + escapedCommand; | |
| } | |
| static std::string escapeAndWrapCommandForShell(const std::string &command) | |
| { | |
| std::string escapedCommand = command; | |
| size_t pos = 0; | |
| // Replace ' with '\'' (close current quote, add literal single quote, open new quote) | |
| while ((pos = escapedCommand.find('\'', pos)) != std::string::npos) { | |
| escapedCommand.replace(pos, 1, "'\\''"); | |
| pos += 4; | |
| } | |
| return "sh -c '" + escapedCommand + "'"; | |
| } |
deepin pr auto review★ 总体评分：20分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 diff --git a/libs/utils/src/linglong/utils/hooks.cpp b/libs/utils/src/linglong/utils/hooks.cpp
index e4eb8d06e..fixed_version 100644
--- a/libs/utils/src/linglong/utils/hooks.cpp
+++ b/libs/utils/src/linglong/utils/hooks.cpp
@@ -35,16 +35,15 @@ constexpr std::string_view POST_UNINSTALL_ACTION_PREFIX = "ll-post-uninstall=";
// This function ensures the command string is safely wrapped for execution via std::system.
static std::string escapeAndWrapCommandForShell(const std::string &command)
{
std::string escapedCommand = command;
size_t pos = 0;
// Replace ' with '\'' (close current quote, add literal single quote, open new quote)
while ((pos = escapedCommand.find('\'', pos)) != std::string::npos) {
escapedCommand.replace(pos, 1, "'\\''");
pos += 4;
}
-
- return "sh -c " + escapedCommand;
+ // std::system 底层自带 sh -c，直接返回被单引号包裹的安全命令即可
+ return "'" + escapedCommand + "'";
}
using CommandList = const std::vector<std::string> &; |
|
@dengbo11: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
2 participants
This reverts commit 9e3220f.